fix: updating SafeToRetry() function to retry on wrapped errors
#141
Conversation
There is a rather unique bug when a single connection is being slammed with queries. When statement caching is enabled, and the `deallocateInvalidatedCachedStatements()` method is called to clean up, the `Pipeline.Sync()` method has the ability to error out due to the connection being in-use. `connLockError` already implements the inexplicit `SafeToRetry() bool()` interface, which is then checked by the `SafeToRetry()` function. In the event the connection is locked due to being in use, the `deallocateInvalidatedCachedStatements()` method can return a wrapped error that looks like: ``` failed to deallocate cached statement(s): conn busy ``` When the `stdlib` PGX wrapper is used, it is possible for this to not be retried, as it is also taking advantage of this `SafeToRetry()` function. In order to correct this and prevent future similar errors, the `SafeToRetry()` function has been updated to check against the wrapped errors as well.
|
Regarding
As far as the actual change goes... Hmmm... Errors are considered unsafe to retry if they do not define a |
|
You know what Jack, I was writing up a long response, but after some rubber duck debugging myself, I realized there was an issue within the application where an existing mutex wasn't blocking all read/write DB operations. That's probably the cause of how I was running into this, derp. In regard to this change, I'll defer to whatever you think is best. To be honest, I did find it a little odd that this was the only place (that I could find) where a retriable error was being wrapped. Per standard Golang conventions, one would typically assume an error wrapped with a Thanks for the assistance here! |
|
On further thought, I think this is a correct change. |
|
I've ported the change to v5 as well. |
|
Nice, thanks for your help! |
There is a rather unique bug when a single connection is being slammed with queries. When statement caching is enabled, and the
deallocateInvalidatedCachedStatements()method is called to clean up, thePipeline.Sync()method has the ability to error out due to the connection being in-use.connLockErroralready implements the inexplicitSafeToRetry() bool()interface, which is then checked by theSafeToRetry()function. In the event the connection is locked due to being in use, thedeallocateInvalidatedCachedStatements()method can return a wrapped error that looks like:When the
stdlibPGX wrapper is used, it is possible for this to not be retried, as it is also taking advantage of thisSafeToRetry()function. In order to correct this and prevent future similar errors, theSafeToRetry()function has been updated to check against the wrapped errors as well.If approved, the change should be made in PGX as well:
https://github.com/jackc/pgx/blob/v5.5.5/pgconn/errors.go#L13-L19